-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement data-map attribute #61
base: main
Are you sure you want to change the base?
Conversation
Would you provide a small usage example? |
@@ -96,6 +96,73 @@ function isEqual(in1, in2) { | |||
|
|||
} | |||
|
|||
// Set a value deep within an arbitrary object | |||
LUX.setDeepObjectValue = function (obj, prop, value) { | |||
//Note: This function was pulled out of Lux |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clarify the meaning of "this function was pulled out of LUX"? Maybe it was pulled from a separate file in the repo? I find it confusing since this still is in the LUX repo.
Also, it is unclear why this is "risky"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a setDeepValue in Lux.js. I didn't want to modify that because i don't know if there are ramifications to the way it was functioning
@@ -96,6 +96,73 @@ function isEqual(in1, in2) { | |||
|
|||
} | |||
|
|||
// Set a value deep within an arbitrary object | |||
LUX.setDeepObjectValue = function (obj, prop, value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting to add a docstring for what these arguments signify
src/HMI/lux-data-bind.js
Outdated
function updateParameterValue($element, localMachine, key, dataVarName) { | ||
let keyname = key.replace('.','-') | ||
if ($element.attr('data-var-name-added-' + keyname) != dataVarName) { | ||
$element.attr('data-var-name-added-' + keyname, dataVarName) | ||
localMachine.initCyclicReadGroup(LUX.getDataReadGroup($element), dataVarName); | ||
} | ||
let value = localMachine.value(dataVarName); | ||
if ($element.attr('data-machine-value-' + keyname) != value) { | ||
$element.attr('data-machine-value-' + keyname, value) | ||
$element.each((index, val) => { | ||
LUX.setDeepObjectValue(val, key, value); | ||
}); | ||
} | ||
} | ||
|
||
function updateParameter($element, elMachine, key, value) { | ||
if (typeof value === 'string') { | ||
updateParameterValue($element, elMachine, key, value); | ||
return | ||
} | ||
|
||
if (typeof value === 'object' && value['data-var-name']) { | ||
let { | ||
['data-var-name']:dataVarName, | ||
machine | ||
} = value; | ||
let localMachine = window[machine] | ||
if(localMachine == undefined){ | ||
localMachine = elMachine; | ||
} | ||
updateParameterValue($element, localMachine, key, dataVarName); | ||
return | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Declaring these everytime the function is called feels wasteful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ya, i was thinking that too, but i also don't want to pollute, where should they go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could declare them on the function LUX.updateDataMaps.updateParameters = function....
example/HMI/views/main.handlebars
Outdated
<input id="testInput" | ||
data-map="{ | ||
'value':{'data-var-name':'testData'}, | ||
'style.backgroundColor':'input.color', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the focus of the PR is on working with web components, can you tell me more about the choice to make this custom mapping attribute, vs leveraging slots on web components?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how slots are relevant, what do you mean? My understanding is that slots are used when you want to integrate user supplied html into the web component, but this is about setting values on the web component
Here is an example usage with a web component:
<ob-rudder-medium data-map="{ 'rudderAngle' : 'tmplitTest:tmplit.Slider' , ' rudderSetPointAngle' : 'tmp1itTest:tmplit.Numericlnput' }"/>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slots could be relevant for uses like input forms, because (I think) the slot could just be one part of the component, with the binding already applied.
<my-fancy-input>
<slot name="userInput" data-var-name="PLC:varToWriteTo" />
<slot name="otherThing" data-var-name="OtherVar"/>
</my-fancy-input>
But on second thought, what is even more relevant than slots is custom attributes on web components. I believe with these you could expose certain component-specific parameters, rather than using an all-in-one mapping. I think it would be much more readable / usable, but would perhaps limit which attributes you could bind to.
Loosely-psuedocoded-example:
<od-rudder-medium data-rudder="tmplitTest:tmplit.Slider" data-rudder-angle="tmplit1Test.NumericInput"/>
The biggest downside I see to the mapping, is readability and how easily one could misspell part of the mapping, and then break all of the mapping for that component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main use case is web-components that we don't build. This is mainly useful for third party components, and means we don't have to consider how the data will map when creating components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that the od-rudder-medium example is from open-bridge
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why it requires internal knowledge of the components, the normal use case would likely not use deep structures. You just have to know what properties to set, which would be the case with any solution.
I don't think we should build data binding into our components, IMO they should really be standalone and not consider integration into lux. The data-map solution covers all use cases, although I agree the json string is not ideal.
I was considering something like
data-lux-[arbitrary property] = "my.pv.name" but I'm not sure this is actually very good from an implementation standpoint, because there aren't great built-in ways to get data-lux* for all elements although that might not be correct.
Can you give any examples of more dev-friendly apis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just tried using document.querySelectorAll('[data-lux*]') and it throws an error that it isn't a valid selector
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only way I see that we could do it is with something like
let elements = Array.from(document.querySelectorAll('*')).filter(el =>
Array.from(el.attributes).some(attr => attr.name.startsWith('data-lux'))
);
but that is really inefficient
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this and it's ~100x slower
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like attributes aren't case sensitive (they are always lower case) so we can't actually support properties with this
What
Implement a new attribute "data-map" that can have a complex json object to map variables to values on an object.
Why
This allows the user to specify more complex databinding than just value. As we use more web components, this will allow us to easily integrate them
Open question:
Currently this writes to the objects properties, but does not set the attributes.. do we want a way to do one or the other? I could image have a prefix on the value to set to specify if it's an attribute or property
I might propose that an @ prefix means attribute?
Example useage: